-
-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add and use usb.ids to show more specific names of USB devices #468
Conversation
861bfae
to
d36b860
Compare
When I run `usbipd list` I see that most [but very interestingly not all] devices have generic not very useful names: ``` C:\path>usbipd.exe list Connected: BUSID VID:PID DEVICE STATE 1-1 048d:8297 USB Input Device Not shared 3-3 0557:2405 USB Input Device Not shared 6-2 0557:2405 USB Input Device Not shared 7-1 046d:085c c922 Pro Stream Webcam, C922 Pro Stream Webcam Not shared 7-2 1b1c:0a14 CORSAIR VOID PRO Wireless Gaming Headset, USB Input Device Not shared 8-1 1b1c:1b8b USB Input Device Shared 8-2 1209:2201 USB Serial Device (COM3), USB Input Device Shared ``` I cannot [easily] tell from this list what devices I want to bind to. Adding and using http://www.linux-usb.org/usb.ids now shows a [I feel] more useful list: ``` C:\path>usbipd.exe list Connected: BUSID VID:PID DEVICE STATE 1-1 048d:8297 Integrated Technology Express, Inc. IT8297 RGB LED Contro... Not shared 3-3 0557:2405 ATEN International Co., Ltd USB Input Device Not shared 6-2 0557:2405 ATEN International Co., Ltd USB Input Device Not shared 7-1 046d:085c Logitech, Inc. C922 Pro Stream Webcam Not shared 7-2 1b1c:0a14 Corsair CORSAIR VOID PRO Wireless Gaming Headset, USB Inp... Not shared 8-1 1b1c:1b8b Corsair USB Input Device Shared 8-2 1209:2201 Generic Dygma Shortcut Keyboard Shared ```
Codecov ReportBase: 41.65% // Head: 39.27% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #468 +/- ##
==========================================
- Coverage 41.65% 39.27% -2.38%
==========================================
Files 28 29 +1
Lines 2612 2770 +158
==========================================
Hits 1088 1088
- Misses 1524 1682 +158
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Hi Paul, Thank you for contributing to usbipd-win!
Conclusion: I can see the benefits of using usb.ids. However, the downside of losing internationalization and deviating from the descriptions in device manager do not warrant an outright replacement. Maybe the descriptions could be used additionally in a new "--verbose" option. Or maybe they could be used if both of the following are true: the driver vendor is Microsoft and there is no FriendlyName associated with the instance (as a kind of heuristic way of figuring out if the Windows name will be too generic). And even then, maybe the usb.ids description should be added, as the generic description will at least be localized. Besides this rationale, there are some issues with the code, but those can be easily fixed. Let's first figure out what we want to achieve here. What problem are we trying to solve? What are the drawbacks of the proposed solution? Do the pros outweigh the cons? PS: This is in no way to discourage you from contributing! In fact, I welcome your input and the fact that your ideas will lead to an improvement of usbipd-win. |
@dorssel Thanks for your comments. I will get back to this soon, since it is an important part of a personal project I am working on. Yes, this PR was just to get the conversation going and at a minimum I will:
Regarding #1, Internationalization, perhaps http://www.linux-usb.org would be willing to crowd source translating those, but most of those entries are proper names of manufacturers and products and the majority of them would not need to be translated. I am certain there are many entries that would benefit from being properly translated, but even if I could only read Greek, Again, I will update this PR with some improvements, worst case just appending the names if a |
Don't worry too much about internationalization. I think we agree that the usb.ids are additional information. If it helps the user: great; if it doesn't, then they will have what they have now: no regression. Localizing usb.ids is a can of worms that I would not open... I like the With this proposal (which is up for discussion, of course) you get exactly what you had in your original screenshots. Except that they are now both available! |
var hardwareId = device.HardwareId; | ||
string description = UsbIds.GetProductName(hardwareId.Vid, hardwareId.Pid, device.Description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be even more useful if we move the whole usb.ids
to the Automation part.
And then make the vendor and product strings accessible directly from the current VidPid class. This makes it not only available in the list
commands, but also from PowerShell and within the state
data.
See "lazy initialization" below for performance considerations.
@@ -0,0 +1,340 @@ | |||
//#define USBIDS_DBG_VERBOSE | |||
#define USBIDS_STOPWATCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry too much about timing. With the --usbids
option, the user has opted in anyway. We'll make is as efficient as possible; best effort.
In any case, please guard this with at least an #ifdef DEBUG
. It should never be enabled for release builds. (or: just take it out).
using System.IO; | ||
using System.Text.RegularExpressions; | ||
|
||
namespace Usbipd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: coding style is namespace Something;
. Saves a whole lot of indenting for the rest of the file.
/// <summary> | ||
/// A C# port of https://github.com/cezanne/usbip-win/blob/master/userspace/lib/names.c (GPLv2+) | ||
/// </summary> | ||
class UsbIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make all non-inherited classes sealed
.
/// </summary> | ||
class UsbIds | ||
{ | ||
static UsbIds() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a "lazy loader".
In fact: how about a static singleton that will trigger the one-off lazy loader?
See RegistryUtils
This allows direct access to a single lookup table, that automatically initializes on first use.
public string name { get; private set; } | ||
} | ||
|
||
private static Dictionary<uint, UsbVendor> vendors = new Dictionary<uint, UsbVendor>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking:
Coding style leaves out default access modifiers. Reason: cleaner code. C# is safe in the sense that it defaults to the most restrictive access. In other words: just leave out private
. You already left out private
in the private (!) nested classes earlier.
Match match; | ||
|
||
string? buf; | ||
while ((buf = f.ReadLine()) != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the original C code to understand how this parser came to be...
Considering that the original is about the worst parser I have ever seen, you actually did a pretty good job improving it. However, we need at least a unit test for this. I'll be refactoring this quite a bit afterwards. For now, it's good enough, provided a unit test is added.
console.WriteTruncated(device.Description, 60, true); | ||
console.WriteTruncated(description, 60, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, add --usbids option. This will then output an alternative description.
I would also change the column header when that option is used, perhaps "USBID" instead of "DEVICE"? (above, at line 130)
|
||
class UsbProduct | ||
{ | ||
public uint vendorid { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# has record
for this. (mentioned once, but true for all these nested classes.
@@ -36,6 +36,9 @@ SPDX-License-Identifier: GPL-2.0-only | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<None Update="usb.ids"> | |||
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | |||
</None> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR (i.e., we can do that in a separate PR), but I like the auto-updater in https://github.com/woodruffw/usb-ids.rs/blob/main/.github/workflows/usbids-updater.yml
Of course, they got the licensing all wrong (!), but we are doing the right thing (GPLv2).
I've created #545 that replaces this MR. |
THIS IS A CONVERSATION PIECE FOR NOW
This work is incomplete, but I wanted to get the conversation going of if this is worth pursing or not.
This does bloat the app a bit with a <800KB <26,000 line file, and the <100ms runtime hit to parse this file, but it I think the pros outweighs these cons.
There might also be a better way to accomplish what this does.
I am extremely surprised that this type of usb id database isn't already built in to Windows 10's or Windows 11's 32GB+ footprint.
When I run
usbipd list
I see that most [but very interestingly not all] devices have generic not very useful names:I cannot [easily] tell from this list what devices I want to bind to.
Adding and using http://www.linux-usb.org/usb.ids now shows a [I feel] more useful list: